Skip to content

Implement TRY_CAST #299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 21, 2021
Merged

Implement TRY_CAST #299

merged 3 commits into from
Mar 21, 2021

Conversation

seddonm1
Copy link
Contributor

Hi @andygrove

This change adds the TRY_CAST alongside CAST so that the choice on how to handle data casting errors can be differentiated.

See: https://docs.microsoft.com/en-us/sql/t-sql/functions/try-cast-transact-sql?view=sql-server-ver15

Also fixed a clippy error that must have been added recently.

@coveralls
Copy link

coveralls commented Mar 17, 2021

Pull Request Test Coverage Report for Build 662686711

  • 24 of 25 (96.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 90.046%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 15 16 93.75%
Totals Coverage Status
Change from base Build 552534083: 0.03%
Covered Lines: 5428
Relevant Lines: 6028

💛 - Coveralls

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @seddonm1

@seddonm1
Copy link
Contributor Author

How does the release process work for this library?

@andygrove
Copy link
Member

How does the release process work for this library?

I'm embarrassed to say that I don't really know anymore. @Dandandan released a version more recently than me so maybe he or @maxcountryman can help with that.

@seddonm1
Copy link
Contributor Author

@andygrove I think we can let you off the hook for not knowing given the work you have done building DataFusion and Ballista 😁

@Dandandan
Copy link
Contributor

@seddonm1 @andygrove

thanks a lot, I will have a look at this PR later.
The release process is documented here: https://github.com/ballista-compute/sqlparser-rs/blob/main/docs/releasing.md
Basically fully automated after doing a version bump.

@seddonm1
Copy link
Contributor Author

Thanks @Dandandan

@maxcountryman
Copy link
Contributor

Happy to help with a release. As @Dandandan mentioned, it should be automated once we've pushed a tag.

@seddonm1
Copy link
Contributor Author

@Dandandan are you able to review and hopefully release? This will allow us to add the CastOptions apache/arrow#9682 to DataFusion - hopefully before the next major release.

@@ -201,6 +201,12 @@ pub enum Expr {
expr: Box<Expr>,
data_type: DataType,
},
/// TRY_CAST an expression to a different data type e.g. `TRY_CAST(foo AS VARCHAR(123))`
// this differs from CAST in the choice of how to implement invalid conversions
TryCast {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering, why the choice TryCast / try_cast?

I know bigquery uses SAFE_CAST https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#safe_casting

It would make sense to support both maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could easily change this to add SAFE_CAST too. There was some rationale discussed here: apache/arrow#9682 (comment)

basically it was just that I think SQL Server got there first with TRY_CAST

@Dandandan
Copy link
Contributor

I think this looks good @seddonm1 ! I think we are good to go, do you want to address the SAFE_CAST question?

@Dandandan Dandandan merged commit e6e37b4 into apache:main Mar 21, 2021
@Dandandan
Copy link
Contributor

@seddonm1 will be released as 0.9.0 , thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants